Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix type def reopening type from parent namespace #11208

Conversation

straight-shoota
Copy link
Member

Resolves #11181

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic labels Sep 14, 2021
@@ -383,6 +383,20 @@ describe "Semantic: class" do
") { char }
end

it "type def does not reopen type from parent namespace (#11181)" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 "type def" is confusing because typedef is a thing in the language. Maybe "type declaration" or just "class def" or "class declaration"?

Copy link
Member Author

@straight-shoota straight-shoota Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took that term from the lookup_type_def method. But yeah, I also thought it might be considered ambiguous.

A type declaration is var : Type, so I don't think that's a better solution. class works here, and I suppose we can use module in the other spec (I didn't add specs for all kinds of types, but I think that's fine b/c the code is identical).

But it would be better to have a generic term for definitions of all kinds of types (class, struct, module, ...). And I think "type definition" is a natural solution for that with "type" being the abstraction for all kinds of types and it being a "definition".

If we go along with #10031 the term would be free from overloads =) For now, I'd propose to leave it as is. In this context, the meaning is unambiguous.

@straight-shoota straight-shoota modified the milestones: 1.12.0, 1.13.0 Apr 2, 2024
@straight-shoota
Copy link
Member Author

straight-shoota commented Apr 4, 2024

This is a bug fix which we could still merge for 1.12.0, but there's a non-dismissible chance for unforseen effects with such a change in the type grammar. This patch has been sitting for years, so there seems to be no rush to release it.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge right after 1.12 is released, so we can have some time in nightly before releasing 1.13 and we don't push it forever.

@straight-shoota straight-shoota merged commit 0efbf53 into crystal-lang:master Apr 13, 2024
58 checks passed
@straight-shoota straight-shoota deleted the fix/type-def-reopen-from-parent-namespace branch April 13, 2024 10:01
cyangle added a commit to cyangle/jennifer.cr that referenced this pull request Aug 8, 2024
cyangle added a commit to cyangle/jennifer.cr that referenced this pull request Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Namespaced type with nested name reopens type in parent namespace
4 participants